http_parser: use `MakeCallback` #5419

trevnorris · 2016-02-24T21:39:40Z

Make HTTPParser an instance of AsyncWrap and make it use
MakeCallback. This means that async wrap hooks will be called on
consumed TCP sockets as well as on non-consumed ones.

Fix: #4416

This PR overrides #4509.

R= @indutny
R= @bnoordhuis

trevnorris · 2016-02-24T21:39:56Z

test/parallel/test-http-parser.js

@@ -104,6 +106,7 @@ function expectBody(expected) {
  assert.throws(function() {
    parser.execute(request, 0, request.length);
  }, Error, 'hello world');
+ */


@indutny Why did you disable this test in your original PR?

Does it pass now?

If I remember it right it just killed the process with uncaught exception last time (FatalException or whatever).

Does on my machine. Running CI again to see if it does.

trevnorris · 2016-02-29T19:03:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1779/

trevnorris · 2016-02-29T19:30:03Z

There are a lot of failures for test-http-pipeline-flood.js. Though this PR shouldn't affect this PR /cc @indutny

trevnorris · 2016-02-29T21:00:56Z

One more time to see if the previous was a fluke

CI: https://ci.nodejs.org/job/node-test-pull-request/1780/

trevnorris · 2016-02-29T23:40:40Z

All but one failure was for test-http-pipeline-flood. Most failures had the usual
AssertionError: Unexpected data received. Though the following had deviations from this:

linux-fips: Error: write ECONNRESET

freebsd: Error: write EPIPE

win10-vcbt2015: FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory

win2012r2-vs2015: # TIMEOUT

@rvagg Maybe you have an idea how to proceed, or maybe something is going on w/ Jenkins?

trevnorris · 2016-03-01T00:13:18Z

@indutny confirmed that switching to MakeCallback() causes the flood protection to fail. figuring out why now.

trevnorris · 2016-03-01T07:25:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1788/

@indutny I've added two AsyncCallbackScopes to prevent the nextTickQueue from being processed. This was causing the state of the parser and socket to be altered, whereas they weren't before. The state change allowed the socket to continually consume new requests.

The reason the pipeline flood fix worked before was because the entire packet of requests was essentially processed synchronously. But calling MakeCallback processes the nextTickQueue and changes the state of the http stream. Below are some of the notes I took while investigating:

In the case of parserOnMessageComplete(), if AsyncCallbackScope is not
used then readStart(parser.socket) will run socket.resume(). This is
because !socket._paused returns true, whereas it shouldn't.

The reason socket._paused = false is because socketOnDrain()
(lib/_http_server.js) is called if the AsyncCallbackScope isn't added.

In the end I believe this fix is the simplest, and keeps the parser operating like it did before.

indutny · 2016-03-01T07:29:34Z

Ah, yeah. Nice catch! I was going to take a look at it, but you beat me 😉

indutny · 2016-03-01T07:29:47Z

LGTM if it works

Make `HTTPParser` an instance of `AsyncWrap` and make it use `MakeCallback`. This means that async wrap hooks will be called on consumed TCP sockets as well as on non-consumed ones. Additional uses of `AsyncCallbackScope` are necessary to prevent improper state from progressing that triggers failure in the test-http-pipeline-flood.js test. Optimally this wouldn't be necessary, but for the time being it's the most sure way to allow operations to proceed as they have. Fix: nodejs#4416 PR-URL: nodejs#5419 Reviewed-By: Fedor Indutny <fedor@indutny.com>

trevnorris · 2016-03-01T09:40:34Z

Thanks much. CI is all green. Landed in a7e49c8.

jasnell · 2016-03-02T16:38:37Z

@trevnorris ... are you going to want to pull this back into v4 at all? If not, can you mark it don't land?

Fishrock123 · 2016-03-02T17:09:49Z

I think the goal is still to backport all the asyncwrap stuff?

Make `HTTPParser` an instance of `AsyncWrap` and make it use `MakeCallback`. This means that async wrap hooks will be called on consumed TCP sockets as well as on non-consumed ones. Additional uses of `AsyncCallbackScope` are necessary to prevent improper state from progressing that triggers failure in the test-http-pipeline-flood.js test. Optimally this wouldn't be necessary, but for the time being it's the most sure way to allow operations to proceed as they have. Fix: #4416 PR-URL: #5419 Reviewed-By: Fedor Indutny <fedor@indutny.com>

trevnorris · 2016-03-02T19:56:12Z

As @Fishrock123 said, all the AsyncWrap changes will need to be back ported if it is to maintain compatibility.

MylesBorins · 2016-03-03T23:31:49Z

Oh hey all. It looks like this commit has created a regression on v5.7.1. Specifically throwing inside of a call back to an http.get request is not throwing :sad:

Here is a gist for the test I was using https://gist.github.com/TheAlphaNerd/6615a27684deb682dfe7

You will notice that the setTimeout is commented out... adding set timeout actually fixes this problem 😢

MylesBorins · 2016-03-10T21:45:11Z

Based on the regression created by this change I'm going to assume we are not going to land it on v4.

@trevnorris let me know if the plans for v4 and asyncwrap changes

rvagg · 2016-03-10T21:51:55Z

I think this probably needs to find its way into LTS but can do so after a longer wait, it fills in the AsyncWrap picture more and the discrepancy between LTS and Stable will be non-trivial if it doesn't land there, and we're getting close to deciding on "official" support for AsyncWrap too.

@thealphanerd I'm going to switch back to lts-watch but we need good settling time for this given the regression.

MylesBorins · 2016-03-10T22:04:02Z

@rvagg SGTM. I'll remove the don't land label from the regression fix

trevnorris · 2016-03-10T22:19:48Z

@thealphanerd The regression was fixed in 3521b05. As @rvagg said this is not absolutely necessary for backport, but it shouldn't be a problem to backport with the fix.

Make `HTTPParser` an instance of `AsyncWrap` and make it use `MakeCallback`. This means that async wrap hooks will be called on consumed TCP sockets as well as on non-consumed ones. Additional uses of `AsyncCallbackScope` are necessary to prevent improper state from progressing that triggers failure in the test-http-pipeline-flood.js test. Optimally this wouldn't be necessary, but for the time being it's the most sure way to allow operations to proceed as they have. Ref: #7048 Fix: #4416 PR-URL: #5419 Reviewed-By: Fedor Indutny <fedor@indutny.com>

trevnorris added http Issues or PRs related to the http subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 24, 2016

trevnorris reviewed Feb 24, 2016
View reviewed changes

trevnorris mentioned this pull request Feb 24, 2016

AsyncWrap: HTTP parserOnBody events happen outside pre/post #4416

Closed

AndreasMadsen mentioned this pull request Feb 25, 2016

[async_hooks] tracking issue nodejs/diagnostics#29

Closed

27 tasks

trevnorris force-pushed the parser-use-mc branch from a39c50d to 7e7d2f3 Compare February 29, 2016 19:02

trevnorris force-pushed the parser-use-mc branch from 7e7d2f3 to 606c839 Compare March 1, 2016 07:16

trevnorris force-pushed the parser-use-mc branch from 606c839 to a7e49c8 Compare March 1, 2016 09:39

trevnorris merged commit a7e49c8 into nodejs:master Mar 1, 2016

trevnorris deleted the parser-use-mc branch March 1, 2016 09:41

Fishrock123 mentioned this pull request Mar 1, 2016

Release proposal: v5.7.1 #5464

Merged

5 tasks

MylesBorins mentioned this pull request Mar 3, 2016

uncaughtException not called with http.get() #5555

Closed

AndreasMadsen mentioned this pull request Mar 4, 2016

AsyncWrap: HTTP has no handle context #3241

Closed

MylesBorins mentioned this pull request Mar 10, 2016

Audit commits not found on v4.x #5647

Closed

MylesBorins added the dont-land-on-v4.x label Mar 10, 2016

rvagg added lts-watch-v4.x and removed dont-land-on-v4.x labels Mar 10, 2016

MylesBorins mentioned this pull request Mar 10, 2016

Track backporting AsyncWrap changes nodejs/Release#86

Closed

AndreasMadsen mentioned this pull request Mar 27, 2016

Fix async wrap http #4509

Closed

stefanmb mentioned this pull request Apr 28, 2016

test: test-http-pipeline-flood failures on Win32 #6450

Closed

AndreasMadsen mentioned this pull request May 29, 2016

Backporting Async Wrap changes #7048

Closed

3 tasks

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 14, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

mscdex mentioned this pull request Dec 19, 2016

node 6 IncomingMessage events fix mscdex/busboy#134

Closed

Clemens-git76 mentioned this pull request Apr 16, 2024

[Snyk] Upgrade rollup from 2.75.6 to 4.13.0 Clemens-git76/node#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http_parser: use `MakeCallback` #5419

http_parser: use `MakeCallback` #5419

trevnorris commented Feb 24, 2016

trevnorris Feb 24, 2016

indutny Feb 24, 2016

trevnorris Feb 29, 2016

trevnorris commented Feb 29, 2016

trevnorris commented Feb 29, 2016

trevnorris commented Feb 29, 2016

trevnorris commented Feb 29, 2016

trevnorris commented Mar 1, 2016

trevnorris commented Mar 1, 2016

indutny commented Mar 1, 2016

indutny commented Mar 1, 2016

trevnorris commented Mar 1, 2016

jasnell commented Mar 2, 2016

Fishrock123 commented Mar 2, 2016

trevnorris commented Mar 2, 2016

MylesBorins commented Mar 3, 2016

MylesBorins commented Mar 10, 2016

rvagg commented Mar 10, 2016

MylesBorins commented Mar 10, 2016

trevnorris commented Mar 10, 2016

http_parser: use MakeCallback #5419

http_parser: use MakeCallback #5419

Conversation

trevnorris commented Feb 24, 2016

trevnorris Feb 24, 2016

Choose a reason for hiding this comment

indutny Feb 24, 2016

Choose a reason for hiding this comment

trevnorris Feb 29, 2016

Choose a reason for hiding this comment

trevnorris commented Feb 29, 2016

trevnorris commented Feb 29, 2016

trevnorris commented Feb 29, 2016

trevnorris commented Feb 29, 2016

trevnorris commented Mar 1, 2016

trevnorris commented Mar 1, 2016

indutny commented Mar 1, 2016

indutny commented Mar 1, 2016

trevnorris commented Mar 1, 2016

jasnell commented Mar 2, 2016

Fishrock123 commented Mar 2, 2016

trevnorris commented Mar 2, 2016

MylesBorins commented Mar 3, 2016

MylesBorins commented Mar 10, 2016

rvagg commented Mar 10, 2016

MylesBorins commented Mar 10, 2016

trevnorris commented Mar 10, 2016

http_parser: use `MakeCallback` #5419

http_parser: use `MakeCallback` #5419